Add "booting" flag and emit "booted" event - #77
Conversation
There was a problem hiding this comment.
app.booting should be reset before calling callback(err).
|
The implementation looks mostly good. Please add a unit-test exercising both sync and async case. |
9f97660 to
c9d0f93
Compare
c9d0f93 to
2bb1157
Compare
7674fbf to
9ef20f5
Compare
|
@bajtos Added unit tests, reassigning to you for review. |
There was a problem hiding this comment.
Please use .to.be.undefined() to prevent jshint warnings.
$ npm test
> loopback-boot@2.5.2 pretest /Users/bajtos/src/loopback/boot
> jscs . && jshint .
No code style errors found.
test/executor.test.js: line 57, col 33, Expected an assignment or function call and instead saw an expression.
test/executor.test.js: line 59, col 27, Expected an assignment or function call and instead saw an expression.
test/executor.test.js: line 60, col 35, Expected an assignment or function call and instead saw an expression.
test/executor.test.js: line 67, col 28, Expected an assignment or function call and instead saw an expression.
test/executor.test.js: line 71, col 27, Expected an assignment or function call and instead saw an expression.
test/executor.test.js: line 77, col 27, Expected an assignment or function call and instead saw an expression.
6 errors
npm ERR! Test failed. See above for more details.
There was a problem hiding this comment.
Didn't see this because I ran mocha instead of npm test. Fixed.
|
see my comments above. |
There was a problem hiding this comment.
This assertion never fails, please remove.
If you feel a test without an assertion is confusing, then add a comment at L67 explaining that the test fails on timeout when the event is not emitted.
|
There is no test checking that Please fix the comment on L67 above and squash all commits into a single one. Other than that, the patch LGTM. |
|
I wanted to add a test for |
196d192 to
df0d8b1
Compare
|
@bajtos Updated with latest comments. Please review again before merge. |
df0d8b1 to
1f7d8e5
Compare
Add "booting" flag and emit "booted" event
|
Landed, thank you. I'll make a new npmjs release a bit later, once I have all pieces related to #86 in place. |
Add a boot script asserting on the flag. // boot/set-app-is-booting.js
module.exports = function(app) {
app.wasBootingInBootScript = app.booting;
}Then run |
2 participants
Connected to strongloop/loopback-workspace#170